chore: Adds deprecation warning on livechat:removeMonitor with new endpoint to replace it
#37168
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughAdds a new REST endpoint POST /v1/livechat/monitors.delete with request/response schemas, updates client and tests to use it, implements server handler calling LivechatEnterprise.removeMonitor, logs deprecation for the old Meteor method, and updates REST typings and test utilities. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37168 +/- ##
===========================================
+ Coverage 68.79% 68.81% +0.01%
===========================================
Files 3361 3361
Lines 114201 114204 +3
Branches 20619 20619
===========================================
+ Hits 78563 78586 +23
+ Misses 33543 33522 -21
- Partials 2095 2096 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Needs #37061 merged before this one can go to review. |
…e/v7/CTZ-77 # Conflicts: # .changeset/many-walls-impress.md # apps/meteor/client/views/omnichannel/monitors/MonitorsTable.tsx # apps/meteor/ee/app/livechat-enterprise/server/api/monitors.ts # apps/meteor/tests/data/livechat/units.ts # apps/meteor/tests/e2e/utils/omnichannel/monitors.ts # apps/meteor/tests/end-to-end/api/livechat/22-monitors.ts # packages/rest-typings/src/v1/omnichannel.ts
lucas-a-pelegrino
added
stat: ready to merge
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 15:38 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 11 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Wed, 03 Dec 2025 15:38:19 GMT |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/ee/app/livechat-enterprise/server/api/monitors.ts (1)
1-16: MissingMeteorimport causes runtime error.The catch block at line 113 references
Meteor.ErrorbutMeteoris not imported in this file.import type { ILivechatMonitor } from '@rocket.chat/core-typings'; import { isPOSTLivechatMonitorCreateRequest, isPOSTLivechatMonitorsDeleteRequest, POSTLivechatMonitorsCreateSuccessResponse, POSTLivechatMonitorsDeleteSuccessResponse, validateBadRequestErrorResponse, validateForbiddenErrorResponse, validateUnauthorizedErrorResponse, } from '@rocket.chat/rest-typings'; +import { Meteor } from 'meteor/meteor'; import { findMonitors, findMonitorByUsername } from './lib/monitors';
🧹 Nitpick comments (1)
apps/meteor/ee/app/livechat-enterprise/server/api/monitors.ts (1)
102-119: Consider usingMeteorErrorfrom@rocket.chat/core-typingsfor consistency.If you prefer not to import Meteor directly, the codebase uses
MeteorErrorfrom@rocket.chat/core-typingsin some places (as seen in theaddMonitorimplementation inLivechatEnterprise.ts). This could be an alternative approach.+import { MeteorError } from '@rocket.chat/core-typings'; // ... in the catch block: - if (error instanceof Meteor.Error) { + if (error instanceof MeteorError) { return API.v1.failure(error.reason); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
apps/meteor/client/views/omnichannel/monitors/MonitorsTable.tsx(3 hunks)apps/meteor/ee/app/livechat-enterprise/server/api/monitors.ts(2 hunks)apps/meteor/ee/app/livechat-enterprise/server/methods/removeMonitor.ts(2 hunks)apps/meteor/tests/e2e/utils/omnichannel/monitors.ts(2 hunks)apps/meteor/tests/end-to-end/api/livechat/22-monitors.ts(2 hunks)packages/rest-typings/src/v1/omnichannel.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/rest-typings/src/v1/omnichannel.tsapps/meteor/ee/app/livechat-enterprise/server/methods/removeMonitor.tsapps/meteor/client/views/omnichannel/monitors/MonitorsTable.tsxapps/meteor/ee/app/livechat-enterprise/server/api/monitors.tsapps/meteor/tests/e2e/utils/omnichannel/monitors.tsapps/meteor/tests/end-to-end/api/livechat/22-monitors.ts
apps/meteor/tests/e2e/**/*.{ts,spec.ts}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,spec.ts}: Store commonly used locators in variables/constants for reuse
Follow Page Object Model pattern consistently in Playwright tests
Files:
apps/meteor/tests/e2e/utils/omnichannel/monitors.ts
🧠 Learnings (6)
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/ee/app/livechat-enterprise/server/methods/removeMonitor.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/ee/app/livechat-enterprise/server/methods/removeMonitor.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/e2e/utils/omnichannel/monitors.tsapps/meteor/tests/end-to-end/api/livechat/22-monitors.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/tests/e2e/utils/omnichannel/monitors.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/e2e/utils/omnichannel/monitors.tsapps/meteor/tests/end-to-end/api/livechat/22-monitors.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/livechat/22-monitors.ts
🧬 Code graph analysis (4)
packages/rest-typings/src/v1/omnichannel.ts (1)
packages/rest-typings/src/v1/Ajv.ts (1)
ajv(24-24)
apps/meteor/ee/app/livechat-enterprise/server/api/monitors.ts (2)
packages/rest-typings/src/v1/omnichannel.ts (4)
POSTLivechatMonitorsCreateSuccessResponse(546-548)isPOSTLivechatMonitorCreateRequest(531-531)POSTLivechatMonitorsDeleteSuccessResponse(575-575)isPOSTLivechatMonitorsDeleteRequest(565-565)apps/meteor/ee/app/livechat-enterprise/server/lib/LivechatEnterprise.ts (1)
LivechatEnterprise(14-186)
apps/meteor/tests/e2e/utils/omnichannel/monitors.ts (1)
apps/meteor/ee/app/livechat-enterprise/server/methods/addMonitor.ts (1)
username(18-27)
apps/meteor/tests/end-to-end/api/livechat/22-monitors.ts (1)
apps/meteor/tests/data/api-data.ts (2)
request(10-10)credentials(39-42)
🔇 Additional comments (8)
packages/rest-typings/src/v1/omnichannel.ts (1)
550-575: LGTM!The new delete request/response type definitions and schemas follow the established patterns in the codebase, with proper validation for the required
usernamefield and consistent success response structure.apps/meteor/tests/e2e/utils/omnichannel/monitors.ts (2)
3-3: LGTM!The
deleteMonitorhelper is concise and correctly uses the new REST endpoint.
17-17:data.usernameis guaranteed to be present in the response.The
monitors.createendpoint queries for the user withusernamein the projection (line 17 of LivechatEnterprise.ts) and the response schemaPOSTLivechatMonitorsCreateSuccessexplicitly includesusernameas a required string field. The endpoint will throw an error if the user is not found, sodata.usernamewill always be available for the delete operation.apps/meteor/client/views/omnichannel/monitors/MonitorsTable.tsx (2)
50-51: LGTM!The endpoint hooks are correctly updated to use the new REST endpoints for monitor management.
98-108: LGTM!The delete handler correctly calls the new endpoint with
{ username }, provides appropriate user feedback via toast messages, and invalidates the query cache to refresh the monitors list.apps/meteor/tests/end-to-end/api/livechat/22-monitors.ts (2)
113-117: LGTM!The test correctly validates successful monitor removal via the new endpoint.
119-128: The test expectation is correct and consistent with the server implementation. TheremoveMonitorendpoint explicitly catches theMeteor.Errorthrown byLivechatEnterprise.removeMonitorand passes onlyerror.reason(which is'Invalid user') toAPI.v1.failure(). This differs from theaddMonitorendpoint, which lacks explicit error handling and allows errors to be caught by a global handler that formats them with the error code suffix. The test at lines 119-128 correctly expects'Invalid user'as the error message.apps/meteor/ee/app/livechat-enterprise/server/api/monitors.ts (1)
65-86: LGTM!The create endpoint refactoring maintains the same behavior while adopting the cleaner chained API pattern with explicit configuration.
apps/meteor/ee/app/livechat-enterprise/server/methods/removeMonitor.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/rest-typings/src/v1/omnichannel.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/rest-typings/src/v1/omnichannel.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
packages/rest-typings/src/v1/omnichannel.ts (1)
550-577: LGTM! Type definitions and schemas follow established patterns.The implementation correctly follows the codebase conventions:
- Request schema properly validates
usernameas a required string fieldadditionalProperties: falseprovides strict validation- Success response uses
ajv.compile<void>which is consistent with other simple success responses in this file (e.g., tags remove at line 606, contact delete at line 1513)- Naming convention matches existing patterns
Proposed changes (including videos or screenshots)
This PR adds a deprecation warning for
livechat:removeMonitorlegacy meteor method, as well as a new endpoint to replace it;livechat/monitors.removeIssue(s)
CORE-1415
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.